Codecov Report

Merging #815 (28c75b0) into main (210e3ac) will increase coverage by 0.26%.
The diff coverage is 80.28%.

@@            Coverage Diff             @@
##             main     #815      +/-   ##
==========================================
+ Coverage   69.76%   70.02%   +0.26%     
==========================================
  Files          41       42       +1     
  Lines        2636     2706      +70     
==========================================
+ Hits         1839     1895      +56     
- Misses        797      811      +14     

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

Is CSV the best thing to use here? Aren't there any other binary-based data formats that can be used instead, and loaded directly into a dataframe...?

Is the problem with CSV "only" that it is not binary, or something else?
At the moment I don't know an easy way to append to file with e.g. JLD2. I believe you would have to reload the dataframe to modify it in place and save it again, which would make the whole thing useless.. Maybe there is a way, but I don't know it. I will look into some of the larger simulation packages (e.g. Oceananigans comes to mind), maybe someone already found a solution to this.

I am surprised that DataFrames.jl does not provide a binary file format that can also be read and written to line-by-line. Seems like the data science people would have figured this out by now.

Yes the problem with CSV is "only" that it is non-binary, but this is a rather big problem. If you are using this functionality, you probably have a lot of data, and to my knowledge text-based ways to save numerical data are the most inefficient in terms of memory. So the disk space you will fill with this will be very large.

Haven't taken time to look for an alternate solution to CSV yet.
But if we agree on the current logic then it will be just a matter of substituting CSV.write with a different write function once an appropriate candidate is found.

I propose to al;ready anticipate the possibility of different backends and provide a keyword argument that configures what backend should be used.

Would something like this work, at the beginning of offline_run!? Other backends could then be included by adding some elseifs, but I have a feeling this might be very bad performance-wise.

write_to_file(filename, data, append) = if lowercase(backend) == "csv"
    AgentsIO.CSV.write(filename, data; append=append)
else
    throw(ArgumentError("Backend $(backend) not supported."))
end

Would this be improved by defining a "maker" function?

function make_writer(backend)
    s = lowercase(backend)
    if s == "csv"
        return (filename, data, append) -> AgentsIO.CSV.write(filename, data; append=append)
    elseif s == "mybackend"
        return (filename, data, append) -> MyBackend.write(filename, data, append)
    else 
        throw(ArgumentError("Backend $(backend) not supported."))
    end
end

and then inside of offline_run! have a line write_to_file = make_writer(backend)?

creating a maker function is indeed a better idea. However you should also introduce a function barrier: after you initialize all data frames and the maker function, pass all of these into another function. This establishes type stability where it matters.

Arrow backend is implemented now. In the process I've condensed the functions a bit. Also added tests for Arrow functionality and they passed locally on my machine.

The workflow for adding a new backend would now be:

• Add writer_$backend function taking filename, data, append as inputs.
• Add backend to get_writer function.
• Done (hopefully). :)

https://github.com/JuliaDynamics/Agents.jl/actions/runs/5333264438/jobs/9663511968?pr=815#step:7:357

Can anyone test this on Windows? I don't have a Windows machine.

yes, you are right, it was a last, really desperate, attempt to solve the problem, don't know why but my Windows didn't complain locally for this :D, anyway I tried to dig up a bit for sometime and I still have no clue why Windows denies the possibility to remove the file

@Tortar Did you run the tests locally on your windows machine and they all passed? If so, then we can be sure that this is a CI windows problem.

no I didn't explain myself well, they passed when I changed the lines with the non sensical approach :D (but this is probably because I also changed something else), they don't pass locally on my Windows machine, the file mdata.arrow can't be removed, but I didn't find any fix

Ah sorry, misunderstood you then. Hm, really weird that it only fails on Windows but has no problems on Linux and MacOS. No idea how to approach this myself, as well, sorry. :/

Sorry for being away but I am overwhelmed with other projects and won't have much time for Agents.jl for the next semester or so... I'll be giving comments if you explicitly as for them by tagging me @Tortar or @fbanning but will have notifications off for the next semester :(

Now, to finish this: if I understand correclty the status quo is that this works fine, but doesn't work on windows, yes? If so, can we then add an error to the first line of the function that checks the OS and prints and error, and open an issue that this function doesn't seem to work as expect in windows with Arrow? In the test suite we only run the test if OS is not windows. Then we can merge? Because everything else seems fine.

If so, can we then add an error to the first line of the function that checks the OS and prints and error, and open an issue that this function doesn't seem to work as expect in windows with Arrow? In the test suite we only run the test if OS is not windows.

Sure thing. I'll add some clauses for that.

Then we can merge? Because everything else seems fine.

I'll do that right after pushing the required OS handling changes and after the tests finally pass.